Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/gis: modernize getl2() #3850

Merged
merged 1 commit into from
Jun 16, 2024
Merged

Conversation

neteler
Copy link
Member

@neteler neteler commented Jun 16, 2024

  • modernize C code in order to simplify code
  • do not fail with different newline types (Linux vs Windows vs ...)

This PR attempts to address CI errors like (source):

ERROR: Cannot open filter file 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\grass8-runneradmin-6736\\tmph6va8xp9'\r\n

- modernize C code to simplify code
- do not fail with different newline types (Linux vs Windows vs ...)

This PR attempts to address CI errors like

`ERROR: Cannot open filter file 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\grass8-runneradmin-6736\\tmph6va8xp9'\r\n`
@neteler neteler added the C Related code is in C label Jun 16, 2024
@neteler neteler self-assigned this Jun 16, 2024
@neteler neteler requested a review from wenzeslaus June 16, 2024 13:53
@neteler neteler added this to the 8.5.0 milestone Jun 16, 2024
@neteler
Copy link
Member Author

neteler commented Jun 16, 2024

@echoix please check if this helps for the Windows tests.

Copy link
Member

@wenzeslaus wenzeslaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shorter using standard functions, possibly faster. I did not find a reason for the current code except that it was trying to avoid usage of string.h. The reason for it likely don't exist anymore.

@echoix
Copy link
Member

echoix commented Jun 16, 2024

Working on it

@echoix
Copy link
Member

echoix commented Jun 16, 2024

The possible time difference measured in the windows CI run seems like a coincidence, maybe by getting allocated a more performant CPU at that time. Some other builds on main and PRs took less time than here, and others took more. So nothing to conclude.

But overall, this code is at least shorter and direct.

@neteler neteler merged commit 88090da into OSGeo:main Jun 16, 2024
26 checks passed
@neteler neteler deleted the libgis_getl2_modernized_C branch June 16, 2024 22:38
a0x8o pushed a commit to a0x8o/grass that referenced this pull request Jun 17, 2024
- modernize C code to simplify code
- do not fail with different newline types (Linux vs Windows vs ...)

This PR attempts to address CI errors like

`ERROR: Cannot open filter file 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\grass8-runneradmin-6736\\tmph6va8xp9'\r\n`
wenzeslaus added a commit that referenced this pull request Sep 13, 2024
Use G_getl2 in G_getl to have everywhere the same behavior of supporting all newlines. Originally, G_getl2 was created to keep the behavior of G_getl. However, now, we want G_getl2 behavior everywhere.

Keeping G_getl2 for compatibility. Code can later be clean up to use only G_getl, probably at the time when G_getl2 is removed (v9).

The new test fails without the change in the library for CRLF for G_getl, but passes with the change. CR fails because it is not supported by fgets used since 88090da (#3850), so expecting failure for CR.

Return and argument types for ctypes fopen need to be set for the code to work everywhere reliably.

---------

Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
Mahesh1998 pushed a commit to Mahesh1998/grass that referenced this pull request Sep 19, 2024
Use G_getl2 in G_getl to have everywhere the same behavior of supporting all newlines. Originally, G_getl2 was created to keep the behavior of G_getl. However, now, we want G_getl2 behavior everywhere.

Keeping G_getl2 for compatibility. Code can later be clean up to use only G_getl, probably at the time when G_getl2 is removed (v9).

The new test fails without the change in the library for CRLF for G_getl, but passes with the change. CR fails because it is not supported by fgets used since 88090da (OSGeo#3850), so expecting failure for CR.

Return and argument types for ctypes fopen need to be set for the code to work everywhere reliably.

---------

Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C Related code is in C libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants